[SYCL][clang-linker-wrapper] Fix argument parsing in Clang Linker Wrapper #20470

YixingZhang007 · 2025-10-25T03:28:30Z

This patch resolves failures in SYCL E2E tests fp64-conv-emu-1.cpp and fp64-conv-emu-2.cpp when using the new offloading model by addressing two issues:

(1) The original implementation in the ClangLinkerWrapper.cpp incorrectly constructed ocloc command line arguments by concatenating all arguments into a single string; this caused parsing failures in the executor. This is fixed in this patch by splitting arguments on whitespace boundaries and rejoining them into a correctly formatted command string.

(2) The double-precision variable test case in fp64-conv-emu-2.cpp has been temporarily disabled because the current implementation only supports -fsycl-fp64-conv-emu, which provides limited FP64 emulation for kernels containing FP64 conversions but no FP64 computations. The test will be re-enabled once -fsycl-fp64-gen-emu (full FP64 emulation) is implemented.

… new offloading model

aelovikov-intel

E2E tests LGTM

maarquitos14 · 2025-10-27T13:54:37Z

clang/lib/Driver/Driver.cpp

+  /// Otherwise return 'false'.
+bool Driver::GetUseNewOffloadDriverForSYCLOffload(Compilation &C,
+                                                  const ArgList &Args) const {
+  // Check only if enabled with -fsycl


Suggested change

// Check only if enabled with -fsycl

// Check only if enabled with -fsycl.

Thank you for your suggestions! The changes for enabling the new offloading model in this PR were copied directly from another PR (#15121). This PR specifically focuses on resolving test failures in fp64-conv-emu-1.cpp and fp64-conv-emu-2.cpp that occur after enabling the new offloading model. I will move any comments related to the new offloading model changes to the original PR where that functionality was implemented. The changes related to enabling the new offloading model have been reverted and removed from this PR.

maarquitos14 · 2025-10-27T13:56:49Z

clang/lib/Driver/Driver.cpp

+  /// Utility function to parse all devices passed via -fsycl-targets.
+  /// Return 'true' for JIT, AOT Intel CPU/GPUs and NVidia/AMD targets.
+  /// Otherwise return 'false'.
+bool Driver::GetUseNewOffloadDriverForSYCLOffload(Compilation &C,


I think the name doesn't describe very well what the function is doing, or at least doesn't align very well with the description in the comment above. I would expect something including the words Get and Devices at least.

Please refer to the previous comment as well :)

maarquitos14 · 2025-10-27T14:38:43Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

+    if (!BeforeOptions.empty()){
+      SmallVector<StringRef, 8> BeforeArgs;
+      BeforeOptions.split(BeforeArgs, " ", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+      for (auto string : BeforeArgs) {


This is most likely introducing a copy for each arg. Can we try const auto & instead?

Thanks for your suggestions! I have made the change.

maarquitos14 · 2025-10-27T14:40:14Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

-      CmdArgs.push_back(Args.MakeArgString(Replace));
+      SmallVector<StringRef, 8> AfterArgs;
+      AfterOptions.split(AfterArgs, " ", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+      std::string JoinedOptions = llvm::join(AfterArgs, " ");


Does this pass all tests? It was similar to this before and I had to add the , because it was causing trouble with ocloc. I added a specific test for that, so if it is passing, then I'm good.

The tests that were previously failing for the clang-linker-wrapper issue (fp64-conv-emu-1.cpp and fp64-conv-emu-2.cpp) are now passing. The tests currently failing in CI (such as https://github.com/intel/llvm/actions/runs/18854382403/job/53800150157) for the new offloading model are unrelated to this issue and will be addressed in future PRs. I am wondering which one is the test you mentioned adding for this issue? I'm not sure if you're referring to https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/SYCLBIN/simple_kernel_aot; this test is passing with the current changes.

maarquitos14 · 2025-10-27T14:41:12Z

sycl/test-e2e/OptionalKernelFeatures/fp64-conv-emu-1.cpp

 // RUN: %clangxx -fsycl -fsycl-targets=intel_gpu_dg2_g10,intel_gpu_dg2_g11,intel_gpu_dg2_g12,intel_gpu_pvc,intel_gpu_mtl_h,intel_gpu_mtl_u -fsycl-fp64-conv-emu %O0 %s -o %t.out
 // RUN: %{run} %t.out

+// RUN: %clangxx -fsycl -fsycl-targets=intel_gpu_dg2_g10,intel_gpu_dg2_g11,intel_gpu_dg2_g12,intel_gpu_pvc,intel_gpu_mtl_h,intel_gpu_mtl_u -fsycl-fp64-conv-emu --offload-new-driver %O0 %s -o %t.out


Could we also try with -g? The code you're changing used to have issues with -g, so just to be on the safe side.

The changes work with '-g' and I have also added a new test command in this file for running the test with -g. Thank you!

srividya-sundaram · 2025-10-28T21:36:56Z

clang/lib/Driver/Driver.cpp

+                   options::OPT_offload_new_driver, false))
+    return false;
+
+  if (Args.hasArg(options::OPT_fintelfpga))


Support for FPGA related options have been removed in this PR.
Don't think this check is necessary.

Thank you for your suggestions! The changes for enabling the new offloading model in this PR were copied directly from another PR (#15121). This PR specifically focuses on resolving test failures in fp64-conv-emu-1.cpp and fp64-conv-emu-2.cpp that occur after enabling the new offloading model. I will move any comments related to the new offloading model changes to the original PR where that functionality was implemented. The changes related to enabling the new offloading model have been reverted and removed from this PR.

sarnex

lgtm but will fall back to marcos for the in-depth review

sarnex · 2025-10-29T15:15:45Z

sycl/test-e2e/OptionalKernelFeatures/fp64-conv-emu-2.cpp

-  if (q.get_device().has(aspect::fp64))
-    nfail += test<Increment<double>>(q);
+  // This test is currently disabled because it requires the -ze-fp64-gen-emu
+  // IGC option to run FP64 arithmetic operations. The -fsycl-fp64-conv-emu flag


sorry why are we seeing this failure now?

sarnex · 2025-10-29T15:16:05Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

-    if (!BeforeOptions.empty())
-      CmdArgs.push_back(BeforeOptions);
+    if (!BeforeOptions.empty()) {
+      SmallVector<StringRef, 8> BeforeArgs;


also just wondering, i see one of your commits is titled says 'revert nick's PR' but i don't remember writing that code so if it is referring to me do you mind linking the PR being reverted, thanks

YixingZhang007 added 2 commits October 21, 2025 06:46

add nicks' commit

edae614

fix the ocloc issue for running the fp64-conv-emu-1.cpp test with the…

6340ea0

… new offloading model

YixingZhang007 requested review from a team as code owners October 25, 2025 03:28

YixingZhang007 requested a review from aelovikov-intel October 25, 2025 03:28

YixingZhang007 marked this pull request as draft October 25, 2025 03:28

YixingZhang007 had a problem deploying to WindowsCILock October 25, 2025 03:28 — with GitHub Actions Failure

got the fp64-conv-emu-1.cpp test to pass

c7bb8d5

YixingZhang007 had a problem deploying to WindowsCILock October 25, 2025 06:22 — with GitHub Actions Failure

YixingZhang007 had a problem deploying to WindowsCILock October 25, 2025 07:01 — with GitHub Actions Failure

aelovikov-intel reviewed Oct 27, 2025

View reviewed changes

maarquitos14 reviewed Oct 27, 2025

View reviewed changes

fix the issue in fp64-conv-emu-2.cpp test

1708433

YixingZhang007 had a problem deploying to WindowsCILock October 27, 2025 18:46 — with GitHub Actions Failure

YixingZhang007 had a problem deploying to WindowsCILock October 27, 2025 19:20 — with GitHub Actions Failure

code clean up

263ce30

YixingZhang007 had a problem deploying to WindowsCILock October 27, 2025 19:33 — with GitHub Actions Failure

resolve CI failure

f8a1c0c

YixingZhang007 had a problem deploying to WindowsCILock October 27, 2025 20:05 — with GitHub Actions Failure

YixingZhang007 had a problem deploying to WindowsCILock October 27, 2025 20:47 — with GitHub Actions Failure

YixingZhang007 mentioned this pull request Oct 28, 2025

Test new offload model #15121

Closed

YixingZhang007 changed the title ~~[NewOffloadModel] Fix argument parsing in Clang Linker Wrapper~~ [SYCL][clang-linker-wrapper] Fix argument parsing in Clang Linker Wrapper Oct 28, 2025

srividya-sundaram reviewed Oct 28, 2025

View reviewed changes

revert Nick's PR

7d853a7

YixingZhang007 had a problem deploying to WindowsCILock October 28, 2025 21:44 — with GitHub Actions Error

resolve clang format failure

aba3bc0

YixingZhang007 temporarily deployed to WindowsCILock October 28, 2025 21:52 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock October 28, 2025 22:41 — with GitHub Actions Inactive

resolve the clang format issue

f50124a

YixingZhang007 temporarily deployed to WindowsCILock October 29, 2025 02:58 — with GitHub Actions Inactive

YixingZhang007 requested review from YuriPlyakhin, maarquitos14, maksimsab and sarnex October 29, 2025 02:59

YixingZhang007 temporarily deployed to WindowsCILock October 29, 2025 03:30 — with GitHub Actions Inactive

YixingZhang007 had a problem deploying to WindowsCILock October 29, 2025 03:30 — with GitHub Actions Failure

YixingZhang007 marked this pull request as ready for review October 29, 2025 04:14

sarnex approved these changes Oct 29, 2025

View reviewed changes

sarnex requested review from a team October 29, 2025 15:17

	// Check only if enabled with -fsycl
	// Check only if enabled with -fsycl.

Uh oh!

[SYCL][clang-linker-wrapper] Fix argument parsing in Clang Linker Wrapper #20470

Are you sure you want to change the base?

[SYCL][clang-linker-wrapper] Fix argument parsing in Clang Linker Wrapper #20470

Conversation

YixingZhang007 commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelovikov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YixingZhang007 Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YixingZhang007 Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YixingZhang007 Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

YixingZhang007 commented Oct 25, 2025 •

edited

Loading

YixingZhang007 Oct 28, 2025 •

edited

Loading

YixingZhang007 Oct 28, 2025 •

edited

Loading

YixingZhang007 Oct 28, 2025 •

edited

Loading